Skip to content

Conversation

Girgias
Copy link
Member

@Girgias Girgias commented Aug 12, 2025

@@ -339,6 +339,7 @@ void zend_oparray_context_begin(zend_oparray_context *prev_context, zend_op_arra
CG(context).try_catch_offset = -1;
CG(context).current_brk_cont = -1;
CG(context).last_brk_cont = 0;
CG(has_assigned_to_http_response_header) = false;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hm, doesn't this need to be on the op array context instead? Otherwise we only warn once per compilation instead of once per file?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've added tests and it seems to work as I expect it

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah okay I see I actually think I wanted to say "once per op_array", but if that's the intention that's fine I guess?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

But it seems to be once per op_array 🤔

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'll check this again tomorrow when I'm more awake

Copy link
Member

@nielsdos nielsdos Aug 25, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see you reset the variable when a new op context starts, but they can be nested. So the following emits 2 deprecations even though it should only emit 1 (artificial example I know, but a simple one):

<?php
function foo() {
    $http_response_header = "foo";
    function nested() {
        echo $http_response_header;
    }
    echo $http_response_header;
}

You can remove the code from "nested" and it will still emit a deprecation that should not be happening.

@Girgias Girgias force-pushed the 8.5-dep-http-response-headers branch from d72a12a to 3b716b4 Compare August 24, 2025 10:04
@Girgias Girgias requested a review from nielsdos August 24, 2025 12:04
@Girgias Girgias marked this pull request as ready for review August 24, 2025 12:04
@Girgias Girgias requested a review from bukka as a code owner August 24, 2025 12:04
@Girgias Girgias requested a review from a team August 24, 2025 22:17
Copy link
Member

@edorian edorian left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

RM approval: 👍

@@ -339,6 +339,7 @@ void zend_oparray_context_begin(zend_oparray_context *prev_context, zend_op_arra
CG(context).try_catch_offset = -1;
CG(context).current_brk_cont = -1;
CG(context).last_brk_cont = 0;
CG(has_assigned_to_http_response_header) = false;
Copy link
Member

@nielsdos nielsdos Aug 25, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see you reset the variable when a new op context starts, but they can be nested. So the following emits 2 deprecations even though it should only emit 1 (artificial example I know, but a simple one):

<?php
function foo() {
    $http_response_header = "foo";
    function nested() {
        echo $http_response_header;
    }
    echo $http_response_header;
}

You can remove the code from "nested" and it will still emit a deprecation that should not be happening.

@Girgias Girgias merged commit 8a5972f into php:master Aug 25, 2025
9 checks passed
@Girgias Girgias deleted the 8.5-dep-http-response-headers branch August 25, 2025 21:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants